feat(eslint): no-unnecessary-use-callback#112689
Conversation
github-actions
bot
added
the
Scope: Frontend
# Conflicts: # static/app/components/replays/table/deleteReplays.tsx
# Conflicts: # static/app/views/explore/metrics/metricToolbar/metricSelector.tsx # static/app/views/performance/landing/metricsDataSwitcherAlert.tsx # static/app/views/settings/seer/overview/codeReviewOverviewSection.tsx
JoshuaKGoldberg
left a comment
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
This is fantastic! 👏 🔥 I'm so pumped to get this in and see the code cleanups. And hopefully eventually get it merged into React lint rule packages upstream.
No blockers, just a few small suggestions that can be taken or leaven.
| if (currentGuide) { | ||
| dismissGuide(currentGuide.guide, step, orgId); | ||
| } | ||
| }; |
There was a problem hiding this comment.
[Non-Blocking] Just noting, handleDismiss is only used once, in a single callback later on that calls it and runs window.location.hash = '';. Another solution could be to move that location setter inside handleDismiss.
...but also not a blocker at all for this PR IMO. Maybe a good other lint rule to noodle on.
| if ( | ||
| node.type === AST_NODE_TYPES.CallExpression && | ||
| node.callee.type === AST_NODE_TYPES.Identifier && | ||
| useCallbackBindings.has(node.callee.name) |
There was a problem hiding this comment.
[Bug] Checking node names is susceptible to bugs caused by variable shadowing. It'd really be better to use
Here's an example commit with four incorrect tests: c148a13
If this were a fully shared open source project I'd request changes. But I don't think the Sentry codebase tends to do this particular shadowing weirdness. IMO it's fine for now.
Co-authored-by: Josh Goldberg ✨ <github@joshuakgoldberg.com>
Sentry Snapshot Testing
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 370dc0e. Configure here.
|
|
||
| if (source.startsWith('@sentry/scraps')) { | ||
| for (const spec of node.specifiers) { | ||
| if (!scrapsExclusions.has(spec.local.name)) { |
There was a problem hiding this comment.
Exclusion check uses local name instead of imported name
Low Severity
The scrapsExclusions check uses spec.local.name (the local alias) instead of the original imported name. For aliased imports like import {CompactSelect as CS} from '@sentry/scraps/compactSelect', the exclusion check against 'CS' won't match 'CompactSelect' in the set, so CS gets added to scrapsImports. The rule would then incorrectly flag useCallback passed to <CS onChange={fn} /> as unnecessary, even though CompactSelect is explicitly excluded because it internally depends on memoized callbacks.
Reviewed by Cursor Bugbot for commit 370dc0e. Configure here.
2 participants
This PR adds a new lint rule,
no-unnecessary-use-callback, that only checks ifuseCallbackis unnecessary in 3 scenarios:This is unnecessary because the referential stability of
fnis never “consumed”.React intrinsic elements like
buttonnever expect props to be memoized, the components themselves are not memoized so theuseCallbackdoes nothing.scrapscomponents. Scraps components, too, don’t expect consumers to memoize input:all other cases are left as they are. Still, there were 194 violations of this new rule, which were auto fixed.